blockchain: allow optimistic block insertion in blockchain #3584
Conversation
| const { header } = block | ||
| const blockHash = header.hash() | ||
| const blockNumber = header.number | ||
| let td = header.difficulty |
There was a problem hiding this comment.
this diff has primarily shifted into the else part of the optimistic condition on L414 with some basic initializations still out side the if {} else {} block
… number to ahash and rebuild canonical while backfilling
g11tech
force-pushed
the
extend-blockchain2
branch
from
f1dba27
to
23fd13f
Compare
| @@ -14,6 +14,8 @@ import { | |||
|
|
|||
| import type { CacheMap } from './manager.js' | |||
|
|
|||
| export type OptimisticOpts = { fcUed: boolean; linked?: boolean } | |||
|
|
|||
There was a problem hiding this comment.
These options are not really "understandable" in a blockchain context respectively bring in concepts from the client (FcU), and a blockchain "should not need to know what an FcU is". 😛
Can we rename this more to the point, so with what this is doing here from the blockchain perspective (I honestly also cannot read this out of the name and bring the two things together)?
Also linked I also can't directly read what is meant here.
These options should - if we keep - also move to types.ts since they become part of the official API eventually.
There was a problem hiding this comment.
(maybe then directly give this some code docs)
… optimistic without moving head but track completion by total difficulty index
|
Don't have anywhere near the full picture, but "total difficulty" sounds dangerous given that post-merge this is not used any more? 🤔 |
packages/blockchain/examples/4444.ts
Outdated
| const common = new Common({ chain: Mainnet, hardfork: Hardfork.Shanghai }) | ||
| // Use the safe static constructor which awaits the init method | ||
| const blockchain = await createBlockchain({ | ||
| validateBlocks: false, // Skipping validation so we can make a simple chain without having to provide complete blocks |
There was a problem hiding this comment.
Ok, so the validateBlocks flag now also allows to insert blocks not being in the canoncial order, is this correct? (or was this behavior already there bofore) I generally do like this! 🙂
So, and - otherway around - if validateBlocks: true, then it remains forbidden to not go the normal path, right (or not), so 1,2,3,4,...?
There was a problem hiding this comment.
Or, not sure if I am fully like it: can't we just fully validate again after this one not validated block (15537393n + 500n). So isn't it only this one block which can't be validated (and therefore would be something like a checkpoint) and then we can (and should) turn validation on again? Or can we do this with the current API already in this example?
There was a problem hiding this comment.
i just picked the flag from other example, but i think yes we can enable the flag, and yes if the parent is missing then it won't be validated, so optimistic inserts will happen
but when you have put an anchor and move the head forward (which again happens through the forward putblocks along the backfilled chain), then they should be validated
i will remove this from the example and see if it works (most probably should)
There was a problem hiding this comment.
Ah, forget about this, this is just simplification for the example, right?
| if (this._validateConsensus) { | ||
| await this.consensus!.validateConsensus(block) | ||
| } | ||
| // check if head is still canonical i.e. if this is a block insertion on tail or reinsertion |
There was a problem hiding this comment.
fyi this function is in need to cleanup so ignore for now
| @@ -13,6 +13,8 @@ import { | |||
|
|
|||
| import type { CacheMap } from './manager.js' | |||
|
|
|||
| export type PutOpts = { canonical?: boolean; parentTd?: bigint } | |||
There was a problem hiding this comment.
Can you move this over to types.ts and give both option some docs?
(also for (my) understanding)
There was a problem hiding this comment.
This is still one of the crucial tasks so that we can work on some better understanding and documentation for these options and eventually come to better names.
So, if we have the definitions as comments over the options, this can be reviewed and alternatives/improvements can be suggested.
There was a problem hiding this comment.
Hi Gajinder, I gave this a look and had a few questions/comments 😄 👍
In general, for both documentation and for review purposes, could you document what this PR is trying to do, and what these new flags are? By the filenames I think this is in the context of EIP-4444. Could you document the new flags in the blockchain, as well as what an Optimistic / Complete block is here in this context? I understand that you are moving the skeleton part away from client and into blockchain, I think that makes sense. However I'm trying to think of how this changes the current behavior of blockchain with these new flags. For instance, what if I insert two optimistic blocks with the same number? What would getBlock(number) then return? In client by backfill we would then create two different subchains - right? How would that work here? 🤔
| }, | ||
| }, | ||
| { common, setHardfork: true }, | ||
| ) |
There was a problem hiding this comment.
Nit: could these be packed in a for loop, and the "magic numbers" (500) be put as a constant?
| await blockchain.putBlock(block3, { canonical: true }) | ||
| headBlock = await blockchain.getCanonicalHeadBlock() | ||
| blockByHash = await blockchain.getBlock(block3.hash()).catch((e) => null) | ||
| blockByNumber = await blockchain.getBlock(block3.header.number).catch((e) => null) |
There was a problem hiding this comment.
We expect that this does not throw - right? So the catch should be removed? (same for blockByHash)
There was a problem hiding this comment.
yes, its just a pattern used to log the status, sometimes blockByNumber will not be found depending upon the scenario
| // 1. We can put any post merge block as 4444 anchor by using TTD as parentTD | ||
| // 2. For pre-merge blocks its prudent to supply correct parentTD so as to respect the | ||
| // hardfork configuration as well as to determine the canonicality of the chain on future putBlocks | ||
| await blockchain.putBlock(block1, { parentTd: chainTTD }) |
There was a problem hiding this comment.
If I were to validateBlocks = true and valdidateConsensus = true, what should happen if I put canonical = true here?
There was a problem hiding this comment.
blockvalidation depends on parent availability in the blockchain and not on any other factor, for optimistic blocks it doesn't happen.
so lets say this was an optimistic block i.e. its parent was not in blockchain, validate blocks will be skipped. if you add canonical=true here, it will add the number => hash index and will run the head update rules (depending on if this is pow block or pos block) since parentTd has been specified and it will assume that it parent chain is confirmed and doesn't need to be added to blockchain
There was a problem hiding this comment.
blockvalidation depends on parent availability in the blockchain
This is too implicit behavior. If we have set validateBlocks = true, blocks should be validated. Period. There shouldn't be inner auto-triggered exceptions from this rule (except maybe for the very explicit task of setting a checkpoint), otherwise one could not rely on the blockchain functionality anymore.
If one wants to deactivate block validation, we should add an option blockchain.setBlockValidation(false) or similar.
Guess this should then be accompanied by a flag for blockchain (if we go this route) to indicate that we might deal with a not-fully validated blockchain.
At least all these things (the "state" of the blockchain) should be transparent and requestable, and not just be "implicitly there".
| // if they are optimistic, i.e. can't apply the normal rule | ||
| // 3. if canonical is not defined, then apply normal rules | ||
| if (opts?.canonical === false || (opts?.canonical === undefined && !isComplete)) { | ||
| if (parentTd !== undefined) { |
There was a problem hiding this comment.
Not sure about the behavior here. If opts.canonical is not set (undefined) then also isComplete is false, so this means that parentTd is undefined. So for PoW jobs blocks would jump to else clause here?
There was a problem hiding this comment.
in this case: (opts?.canonical === undefined && !isComplete) would evaluate to true and the block will just be added to the blockchain as optimistic block, and no head update rules will be run or number => hash index modified
| if ( | ||
| !block.isGenesis() && | ||
| block.common.consensusType() !== ConsensusType.ProofOfStake && | ||
| opts?.canonical === undefined |
There was a problem hiding this comment.
This will not work if opts.canonical is set to true, even if we don't have the parentTd, is this intended?
There was a problem hiding this comment.
if parentTD is undefined, and this is not an explicity specified canonical PoS block, throw error because we can't determine the canonicality of this block
ideally such a scenario will not happen becuase we will not end up in else part of condition of L424, i will do a cleanup and see if we can come to a simplified logic, it could be redundant
| await blockchain.putBlock(block).catch((_e) => { | ||
| undefined | ||
| }) | ||
| dbBlock = await blockchain.getBlock(block.hash()).catch((_e) => undefined) |
There was a problem hiding this comment.
I'm not entirely sure if I like this test setup. We expect dbBlock is undefined, but this can happen in 2 cases: (1) getBlock returns undefined and (2) getBlock throws. I think we should explicitly try/catch this and check for the expected behavior (either the error, or that it returns undefined).
There was a problem hiding this comment.
true but since the condition below checks for it to be not undefined, the reason doesnt matter for the purposes of the test and will need to be debugged
| ) | ||
|
|
||
| const td = await blockchain.getTotalDifficulty(block.hash()).catch((_e) => undefined) | ||
| assert.equal(td, undefined, 'block should be not be marked complete') |
There was a problem hiding this comment.
What is a "complete" block?
There was a problem hiding this comment.
a complete block is the one which is confirmed to be linked all the way to the genesis via chain of parent blocks.
in our blockchain db, this is marked via block having the totalDifficulty written onto the chain. a boolean flag could also be used, but total difficulty serves well for both PoW and PoS blocks since PoW total difficulty is actually need to make head and canonicality decisions, while for PoS it just acts an an indicator.
while putting a block, if block's parent is in the blockchain and the parent also has totalDifficulty, then the new block is considered complete and it can move head
There was a problem hiding this comment.
We should really really define (write down!) very clearly what we mean with both canonical and complete here. Again, maybe we are also "not there" with these names yet (e.g. complete for a block with this kind of definition is very misleading, one is just thinking in the realm of that the block has all txs, fields, .... We need to urgently find some better and more precise terminology here).
| // 1. We can put any post merge block as 4444 anchor by using TTD as parentTD | ||
| // 2. For pre-merge blocks its prudent to supply correct parentTD so as to respect the | ||
| // hardfork configuration as well as to determine the canonicality of the chain on future putBlocks | ||
| await blockchain.putBlock(block1, { parentTd: chainTTD }) |
There was a problem hiding this comment.
I still do not fully understand this anchor concept. What is anchored and how?
There was a problem hiding this comment.
block1 is the 4444 anchor i.e. the parent chain to block1 doesn't need to be added to the blockchain and blockchain can be build forward from here
it acts as a re-genesis anchor, one specifies an anchor by providing parentTD, for a PoS block parentTD can just be chainTTD, for a PoW block an actual parentTD should be added so as to eventually and correctly matchup with the terminal block TTD when forward chain blocks will be added on top of it
There was a problem hiding this comment.
These are one of the things which are too implicit, at least on the API, maybe the DB level. I am still not fully grasping if the parentTd is also needed (for calculating/validating something) or is only set in some way to indicate the anchor status.
No matter how it is: this is unnecessarily implicit and we should think about the "anchor" (or call it checkpoint?) status and give this its own API and database representation.
I think "Checkpoint" might actually be the more common and established terminolgy.
So I think we definitely want to have some external relation in the DB where we store all the checkpoints (and not only have this in some very implicit form), and can e.g. extend our API with things like:
blockchain.getCheckpoints()
blockchain.isCheckpoint(5n)(or first there would be the question if there can be only one of these, but guess there can be several respectively would make sense?)
(wonder if we can even lign in the genesis block in this concept, guess this is exactly the same as a checkpoint?)
And then for setting these checkpoints, this should be very explicitly named. So if parentTd is still necessary, even if we store a Checkpoints -> [ Block Numbers ] relation, then this should minimally be provided with naming the option setCheckpoint: 560934n (and so provide the parentTD still, but set the very emphasis on this checkpoint setting, so one can read from the API call what is happening here).
| if (!block.isGenesis()) { | ||
| td += parentTd | ||
| } | ||
| // 1. if canonical is explicit false then just dump the block |
There was a problem hiding this comment.
What does canonical === false mean here? Just that we got a block that we know isn't canonical but we want to hold on to it "for future reference"?
| let updatesHeadBlock | ||
| if (parentTd === undefined) { | ||
| // if the block is pow and optimistic, and has not been explicity marked canonical, then | ||
| // throw error since pow blocks can't be optimistically added without expicit instruction about |
There was a problem hiding this comment.
I don't think I grasp what this means. How do we know if a PoW block is canonical if we don't have the chain of blocks with successively greater TD leading up to it? Presumably we aren't communicating with a CL client in this instance so not sure where this notion of canonicality would come from external to the blockchain class.
re-imagining the previous work (#3548) of extending blockchain for skeleton use based on the feedback